-
Notifications
You must be signed in to change notification settings - Fork 274
split up remove_function_pointerst::remove_function_pointer #2870
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
const code_function_callt &code = to_code_function_call(target->code); | ||
|
||
const exprt &function = code.function(); | ||
const exprt &pointer = function.op0(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could cast to dereference_exprt
and then use .pointer()
to make explicit that we're expecting a dereference expression here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
void remove_function_pointer( | ||
goto_programt &goto_program, | ||
goto_programt::targett target, | ||
const functionst &functions); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could be protected
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Trying to expose this, for alternative ways of determining those functions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Surely a good idea, but now we have two functions with the same name, and neither of which are documented. I don't think RTFS makes for good-enough documentation in this case. Please add a brief documentation header to the new one at least (as it is also publicly exposed), or even better: to both of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Doxygen-style formatting for the comment, otherwise ok.
Done |
const code_function_callt &code = to_code_function_call(target->code); | ||
|
||
const exprt &function = code.function(); | ||
PRECONDITION(function.id()==ID_dereference); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-format
This splits up the largest method in the class into two pieces; there is a natural cut put, which is then also exposed as a public interface.
ef87ddd
to
10d8b2f
Compare
This splits up the largest method in the class into two pieces;
there is a natural cut point, which is then also exposed as a public
interface.